[WIP] Implement RFC 41: lib.fixed#1578
Conversation
|
I noticed from amaranth import *
from amaranth.lib import fixed
from amaranth.lib.wiring import Component, In, Out
class C(Component):
o: Out(1)
def elaborate(self, platform):
m = Module()
self.s = Signal(2)
self.f = Signal(fixed.SQ(2, 2))
m.d.sync += self.o.eq(self.s)
return m
from amaranth.sim import Period, Simulator
dut = C()
async def bench(ctx):
for _ in range(1):
print(ctx.get(dut.s) == 0) # True
print(ctx.get(dut.f) == 0) # (== (s (slice (const 4'sd0) 0:4)) (cat (const 2'd0) (const 1'd0)))
await ctx.tick()
sim = Simulator(dut)
sim.add_clock(Period())
sim.add_testbench(bench)
sim.run()outputs: |
|
@goekce Thanks for taking a look! I think this is happening because of the comparison outside the simulation context, rather than inside it, which means that As an example for writing assertions, maybe take a look at the tests attached to this PR. That being said, this is still a work in progress and not quite ready for review yet! |
|
@goekce a related topic to your example's evaluation outside the simulation context is elaboration-time evaluation of constant expressions. which is something I'd like to leave out of this RFC, even if we could add it in the future. For example |
|
I was not aware of the possibility that I can do a simulation-time comparison. Thanks Seb, this would solve my problem. As I understand, elaboration-time evaluation of constant expressions is extra work.👍 |
|
You can already do elaboration-time comparison of const expressions by doing |
|
For comparison with zero that makes sense but for other values, if we drop the I think this kind of use case is better covered by In the example from @goekce - if we really want to do the comparison outside the sim context for some reason |
|
What I mean is that |
|
I am confused about how I thought But it is probably not used: Or do you just want to say that |
|
If a
from amaranth import *
from amaranth.lib import fixed
from amaranth.lib.wiring import Component, In, Out
class C(Component):
y: Out(fixed.SQ(8, 4))
def elaborate(self, platform):
m = Module()
self.a = Signal(self.y.shape())
self.b = Signal(self.y.shape())
self.c = Signal(self.y.shape())
m.d.comb += [
self.a.eq(fixed.Const(-1.125) + Mux(1, fixed.Const(1.375), 0)),
self.b.eq(fixed.Const(-1.125) + fixed.Const(1.375)),
self.c.eq(Mux(1, fixed.Const(1.375), 0)),
]
m.d.sync += self.y.eq(self.a)
return m
from amaranth.sim import Period, Simulator
dut = C()
async def bench(ctx):
await ctx.tick()
print(ctx.get(dut.a).as_float())
print(ctx.get(dut.b).as_float())
print(ctx.get(dut.c).as_float())
print(Mux(1, fixed.Const(1.375), 0).shape())
sim = Simulator(dut)
sim.add_clock(Period())
sim.add_testbench(bench)
sim.run()Result: |
Co-authored-by: Gökçe Aydos <18174744+goekce@users.noreply.github.com>
Thanks, I haven't played with Ideally we want to attack this without touching anything outside It seems other types similarly lose the type information through In this case it seems making |
Yeah, that sounds reasonable. We actually discussed the option of preserving shapes through |
If ...
self.c.eq(Mux(1, fixed.Const(1.375), 0)),
self.d.eq(Mux(1, fixed.Const(1.375, self.y.shape()), 0)),
self.e.eq(fixed.Const(-1.125) + Mux(1, fixed.Const(1.375, self.y.shape()), 0)),
self.f.eq(fixed.Const(-1.125, self.y.shape()) + Mux(1, fixed.Const(1.375, self.y.shape()), 0)),
self.g.eq(fixed.Const(-1.125, self.y.shape()) + Mux(1, fixed.Const(1.375, self.y.shape()), fixed.Const(0, self.y.shape()))),
...
print(ctx.get(dut.d).as_float())
...Outputs: The workaround I found was to use an |
If one really wants to use a On your example: I would however not include such a workaround in this RFC. I think attacking this properly would imply modifying Whether |
|
Thanks for the IMHO |
|
| def reshape(self, f_bits): | ||
| # If we're increasing precision, extend with more fractional bits. If we're | ||
| # reducing precision, truncate bits. | ||
| shape = hdl.Shape(self.i_bits + f_bits, signed=self.signed) | ||
| if f_bits > self.f_bits: | ||
| result = Shape(shape, f_bits)(hdl.Cat(hdl.Const(0, f_bits - self.f_bits), self.as_value())) | ||
| elif f_bits < self.f_bits: | ||
| result = Shape(shape, f_bits)(self.as_value()[self.f_bits - f_bits:]) | ||
| else: | ||
| result = Shape(shape, f_bits)(self.as_value()) | ||
| return result |
There was a problem hiding this comment.
I additionally noticed that reshape(shape) from the RFC is not implemented. Is this intended?
Agree this is an edgecase in const size inference. Let me add a test case for that and fix. Thanks! 👍
Yes this is intended, my plan was to change the RFC text to reflect this, as I couldn't find a good usecase for the second form of |
| def reshape(self, f_bits): | ||
| # If we're increasing precision, extend with more fractional bits. If we're | ||
| # reducing precision, truncate bits. | ||
| shape = hdl.Shape(self.i_bits + f_bits, signed=self.signed) | ||
| if f_bits > self.f_bits: | ||
| result = Shape(shape, f_bits)(hdl.Cat(hdl.Const(0, f_bits - self.f_bits), self.as_value())) | ||
| elif f_bits < self.f_bits: | ||
| result = Shape(shape, f_bits)(self.as_value()[self.f_bits - f_bits:]) | ||
| else: | ||
| result = Shape(shape, f_bits)(self.as_value()) | ||
| return result |
There was a problem hiding this comment.
reshape doesn't work when self.i_bits == 0 and f_bits == 0.
This may happen with unsigned, fraction only types.
As this would just always return a 0-value it may be of questionable use, but for the sake of consistency I think it should work.
Furthermore this breaks eq when assigning a value with no integer part to a signal with on fractional part. In that case I think it should either assign constant 0 or throw an exception
There was a problem hiding this comment.
Furthermore this breaks eq when assigning a value with no integer part to a signal with on fractional part. In that case I think it should either assign constant 0 or throw an exception
We already throw an exception in this case. Currently I get:
>>> Signal(fixed.UQ(2, 0)).eq(fixed.Const(0, fixed.UQ(0, 2)))
amaranth/lib/fixed.py:124: in eq
other = other.reshape(self.f_bits)
^^^^^^^^^^^^^^^^^^^^^^^^^^
amaranth/lib/fixed.py:134: in reshape
result = Shape(shape, f_bits)(self.as_value()[self.f_bits - f_bits:])
^^^^^^^^^^^^^^^^^^^^
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = UQ(0, 0), shape = unsigned(0), f_bits = 0
def __init__(self, shape, f_bits=0):
self._storage_shape = shape
self.i_bits, self.f_bits = shape.width-f_bits, f_bits
if self.i_bits < 0 or self.f_bits < 0:
raise TypeError(f"fixed.Shape may not be created with negative bit widths (i_bits={self.i_bits}, f_bits={self.f_bits})")
if shape.signed and self.i_bits == 0:
raise TypeError(f"A signed fixed.Shape cannot be created with i_bits=0")
if self.i_bits + self.f_bits == 0:
> raise TypeError(f"fixed.Shape may not be created with zero width")
E TypeError: fixed.Shape may not be created with zero width
Although I agree that it may be worth adding a check for this inside reshape, to give the user a more helpful error message a bit earlier rather than falling through to the Shape constructor.
I would prefer to raise a more useful error message in these cases, rather than permit an operation that has questionable / no real use. Unless there is some killer reason to permit it?
There was a problem hiding this comment.
I don't really see why are 0-width unsigned fixed point values prohibited, as the standard signals allow for such a case (unsigned(0) is OK, but `signed(0) is not).
I don't know how frequently it would show itself, but something like this triggers the problem:
v = Signal(fixed.UQ(1,0))
m.d.sync += v.eq(v >> 1)It turns out there are some more problems with zero-wide assignments to signed fixed point values:
v = Signal(fixed.SQ(1,0))
m.d.sync += v.eq(v >> 1)It seems like there is more fundamental problem with shifts by signal:
x = Signal(range(8))
v = Signal(fixed.UQ(10,0))
assert (v >> x).shape() == fixed.UQ(3,7) # should be UQ(10,7)The left shifts don't suffer the same problems.
This was why I even found the reshape problem (unsiged fixed got right-shifted by a signal and assigned)
I think I have fixed all of that in qbojj/PixelForge@23244b8
|
Thanks @qbojj for taking a look at this! I'm currently out, so my responses are a bit slow for a couple weeks, but just wanted to mention I have read through your comments and will integrate them into this PR shortly. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1578 +/- ##
==========================================
- Coverage 91.32% 91.07% -0.26%
==========================================
Files 44 45 +1
Lines 11389 11726 +337
Branches 2219 2281 +62
==========================================
+ Hits 10401 10679 +278
- Misses 827 869 +42
- Partials 161 178 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I don't know how usefull it would be for others, but from time to time I was using rounding like: def reshape_round(self, f_bits):
# Reshape with rounding when reducing precision.
if f_bits >= self.f_bits:
return self.reshape(f_bits)
else:
r = self.reshape(f_bits)
do_inc = self.as_value()[self.f_bits - f_bits - 1]
return r.shape()(r.as_value() + do_inc)
def reshape_ceil(self, f_bits):
# Reshape with ceiling when reducing precision.
if f_bits >= self.f_bits:
return self.reshape(f_bits)
else:
r = self.reshape(f_bits)
do_inc = self.as_value()[self.f_bits - f_bits - 1 :] != 0
return r.shape()(r.as_value() + do_inc)
def truncate_round(self, f_bits=0): ...
def truncate_ceil(self, f_bits=0): ...
def floor(self) -> hdl.Value:
return self.truncate(0).as_value()
def ceil(self) -> hdl.Value:
return self.truncate_ceil(0).as_value()
def round(self) -> hdl.Value:
return self.truncate_round(0).as_value()PS. all my code samples are from qbojj/PixelForge/gpu/utils/fixed.py, including semi-capable formatter |
Overview
lib.fixedwhile the associated RFC is being worked on. It started as a fork of @zyp's early implementation of the RFC here, however a few things have changed since then.fixed.Value-.saturate()and.clamp()- these are commonly needed in my DSP codebase, but it may make sense to punt these new methods to a future RFC..truncate()is added as an alias for.reshape(), the only difference being that it verifies there was a reduction off_bitsrequested.numerator()method is dropped, as I found a way to combine it withas_value()reliably.lib.fixedin this Tiliqua PR and tested it underneath my library of DSP cores, and will continue to use the learnings there in order to guide the RFC.Simple example
Consider the implementation of a simple Low-pass filter, where we wish to compute the difference equation
y = y[n-1] * beta + x * (1 - beta)using fixed point representation: